Skip to content

perf: render Overview row selection on the GPU to fix scroll stutter (#1674)#1740

Merged
steipete merged 3 commits into
steipete:mainfrom
hhh2210:codex/overview-gpu-selection
Jun 28, 2026
Merged

perf: render Overview row selection on the GPU to fix scroll stutter (#1674)#1740
steipete merged 3 commits into
steipete:mainfrom
hhh2210:codex/overview-gpu-selection

Conversation

@hhh2210

@hhh2210 hhh2210 commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Third direction for #1674, superseding the lite-row (#1675) and rich-row (#1676) drafts: keep the rich Overview rows, but move selection rendering off the SwiftUI graph onto the GPU. Two parallel explorations (Claude + Codex) independently converged on this design.

Root cause (measured)

Overview rows are rich SwiftUI views hosted in NSMenu. Each scroll step flips menuItemHighlighted, which re-rasterizes the whole row subtree. A headless benchmark drove the real production hosting path (OverviewMenuCardRowView → MenuCardSectionContainerView → hosting view), toggling selection 200× (setHighlighted → layout → display → runloop-flush; macOS 27, Swift 6.4, debug):

Variant avg / toggle max Verdict
A current SwiftUI recolor ~3.0 ms ~10.6 ms spikes blow the 8.3 ms / 120 Hz budget
B + .drawingGroup() (Metal rasterization only) ~2.2 ms ~9.6 ms only ~28% — Metal alone insufficient
C highlight fully decoupled (floor) ~0.01 ms — theoretical floor
E GPU CIColorMatrix + AppKit selection (this PR) 0.044 ms 1.39 ms 53× faster, within 120 Hz

Before/after on the production types, same loop/machine:

OLD SwiftUI selection      avg = 2.354 ms   max = 7.287 ms
NEW AppKit/GPU selection   avg = 0.044 ms   max = 1.386 ms

This lines up with the report: the reporter's recording is a 120 fps capture running ~57 fps effective with worst frame gaps ~25 ms, matching variant A's spikes. Variant D (content pinned, only the container's highlight modifiers change) still cost ~3–8 ms, proving the cost is content re-rasterization on highlight — so the fix must take selection off the SwiftUI graph.

Fix

Two GPU-composited steps, zero SwiftUI work per scroll step:

  1. AppKit NSVisualEffectView(.selection) selection background (the existing PersistentRefreshMenuView pattern), crossfaded via Core Animation.
  2. A CIColorMatrix content filter mapping the row to the selected text color — Core Image runs on the GPU (Metal), matching the existing design where selected rows already go white.

Scoped to Overview rows via makeMenuCardItem(usesGPUSelection:); it does not override hitTest, so embedded controls (e.g. the error copy button flagged on #1676) keep working.

Hand-feel (不跟手) — separate, also addressed

Cheap highlight toggles don't fix feel: handleOverviewScrollWheel quantizes the wheel into discrete row jumps. Precise trackpad events are now passed through to AppKit's native menu scroller (continuous, finger-following); classic notched wheels keep row navigation.

Verification (timing-independent)

gpu selection highlight bypasses swiftui highlight state: highlighting a GPU row marks the AppKit view selected while the hosted MenuCardHighlightState.isHighlighted stays false — proving selection never re-invalidates the SwiftUI graph. swift build clean; Overview-scroll + menu-card recycling/highlight suites green.

Absolute ms are debug-build / load dependent; the relative 53× delta and the cross-budget A↔E gap are the load-independent signal. Interactive 120 Hz feel still benefits from a real-device ProMotion confirmation.

See docs/overview-scroll-stutter-investigation.md for the full analysis.
Adding the local recording that motivated the final GPUSelectionHostingView + precise-trackpad pass-through direction.

Scope: this is a baseline / problem recording, captured before the GPU-selection + native precise-scroll fix in this PR. It shows the Overview menu feeling non-finger-following / stuttery while scrolling a provider-heavy rich-row menu — repro evidence for #1674, and context for why this PR also passes precise trackpad deltas through to AppKit's native menu scrolling. The clip is cropped to remove account identifiers; the original local capture was 120 fps H.264.

The proof for the patch itself remains the benchmark + tests in the PR body:

codexbar-overview-scroll-proof-public-crop.mp4 
E:gpu-selection-actual  avg=0.041ms  p50=0.049ms  p95=0.068ms  max=0.112ms
swift build
swift test --filter StatusMenuOverviewScrollTests
make check

A follow-up real-menu recording from the packaged build would be the strongest end-to-end proof.

🤖 Generated with Claude Code

@hhh2210 @claude
perf: render Overview row selection on the GPU to fix scroll stutter
b6d0327 
Overview rows are rich SwiftUI views hosted in NSMenu. Every scroll step
flipped `menuItemHighlighted`, which re-rasterized the whole row subtree
(~2.4 ms avg, ~25 ms spikes — matching the dropped frames reported in steipete#1674).
A headless benchmark over the real hosting path measured A: SwiftUI recolor
avg 2.35 ms / max 7.29 ms; `.drawingGroup()` (Metal rasterization only) helped
~28% and still missed the 8.3 ms/120 Hz budget.

Move selection rendering off the SwiftUI graph entirely:
- AppKit NSVisualEffectView(.selection) background, crossfaded via Core Animation
- a CIColorMatrix content filter tinting the row to the selected text color,
  which Core Image runs on the GPU (Metal) and matches the existing all-white
  selected look

Per-toggle cost drops to ~0.044 ms / max 1.39 ms — 53x faster, well within the
120 Hz budget. Scoped to Overview rows via `makeMenuCardItem(usesGPUSelection:)`;
it does not override hitTest, so embedded controls keep working.

Also pass precise trackpad scrolls through to AppKit's native menu scroller
instead of thresholded row-highlight jumps, so the menu follows the finger;
classic notched wheels keep row-to-row navigation. (Trackpad pass-through and
the deterministic bypass test came from a parallel Codex exploration.)

Adds `gpu selection highlight bypasses swiftui highlight state`, a timing-free
test proving the GPU selection never invalidates the hosted SwiftUI highlight
state.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 24, 2026 14:16
@clawsweeper

clawsweeper Bot commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed June 28, 2026, 6:07 AM ET / 10:07 UTC.

Summary
The PR adds a GPU/AppKit selection host for Overview menu rows, routes Overview rows through it, changes precise trackpad scrolling to native AppKit scrolling, adds focused tests, and adds an investigation document.

Reproducibility: no. high-confidence local reproduction was run in this read-only review. The linked issue, sample fragments, benchmark claims, and current source make the Overview NSMenu/SwiftUI highlight path source-reproducible, but real packaged runtime behavior still needs visual or profiling proof.

Review metrics: 3 noteworthy metrics.

  • PR diff surface: 7 files changed, +862/-43. The patch crosses menu hosting, scroll handling, tests, and docs, so runtime proof matters beyond compile checks.
  • Related fix candidates: 3 open PRs for the same report. Maintainers need to avoid merging mutually exclusive Overview stutter directions.
  • Proof artifacts inspected: 2 downloaded, 2 metadata-probed, 0 visual frames verified. The environment lacks video frame extraction tools, so proof confidence cannot rely on visual inspection.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🦞 diamond lobster
Result: blocked until stronger real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add redacted after-fix packaged-build proof showing provider-heavy Overview scrolling, row selection, provider click, and provider-detail submenu behavior.
  • Redact private information such as account identifiers, local paths, API keys, phone numbers, and non-public endpoints before posting proof.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: A baseline recording, a later short video, and benchmark/test output are present, but no inspected artifact identifies a freshly packaged after-fix build and covers scrolling plus interaction behavior. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Mantis proof suggestion
A visible desktop proof would materially help verify the changed CodexBar menu scrolling, selection, click, and submenu behavior in a real app session. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis visual task: verify a freshly packaged CodexBar build scrolls provider-heavy Overview smoothly and row selection, provider click, and provider-detail submenu still work.

Risk before merge

  • [P1] The available media could only be metadata-inspected here, so it still does not prove a freshly packaged after-fix Overview menu with scrolling, selection, provider click, and provider-detail submenu behavior.
  • [P1] Merging changes precise-trackpad Overview scrolling from row-step selection movement to native AppKit menu scrolling, so existing users may experience different selection and submenu behavior on upgrade.
  • [P1] The PR adds a new NSView/Core Animation/Core Image host on a primary NSMenu path; focused tests help, but they do not fully prove real menu tracking, layer filtering, accessibility press, click, and submenu behavior on macOS 27.
  • [P1] Two sibling candidate PRs for the same report remain open, so maintainers should explicitly choose the intended Overview stutter direction and close or park the alternatives.

Maintainer options:

  1. Verify The Real Menu Path (recommended)
    Before merge, confirm a freshly packaged build shows provider-heavy Overview scrolling plus selection, provider click, and provider-detail submenu behavior after the GPU-selection change.
  2. Choose This Direction Explicitly
    If maintainers accept the GPU-selection and precise-scroll behavior change, close or park the compact-row and earlier rich-row alternatives so one fix path owns the report.
  3. Pause If Proof Stays Ambiguous
    If the available media cannot be confirmed as after-fix packaged behavior, keep this PR open rather than merging primary menu rendering changes on benchmark output alone.

Next step before merge

  • [P1] The remaining blocker is maintainer/runtime validation and direction choice, not a narrow automated code repair.

Security
Cleared: The diff touches local AppKit/SwiftUI menu rendering, tests, and docs; no dependency, workflow, secret, or build-system security concern was found.

Review details

Best possible solution:

Land one selected Overview stutter fix after maintainer-visible macOS packaged-build proof confirms scrolling, selection, click, and submenu behavior, then close or park the alternative PRs.

Do we have a high-confidence way to reproduce the issue?

No high-confidence local reproduction was run in this read-only review. The linked issue, sample fragments, benchmark claims, and current source make the Overview NSMenu/SwiftUI highlight path source-reproducible, but real packaged runtime behavior still needs visual or profiling proof.

Is this the best way to solve the issue?

Unclear. Moving selection off the SwiftUI graph is a coherent narrow fix and the maintainer appearance pass addresses a concrete tint concern, but the precise-scroll UX change and competing PRs still need maintainer direction plus runtime proof.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 18eab6b68e3e.

Label changes

Label changes:

  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦞 diamond lobster.
  • remove rating: 🦐 gold shrimp: Current PR rating is rating: 🦪 silver shellfish, so this older rating label is no longer current.

Label justifications:

  • P2: This is a normal-priority performance and interaction fix candidate for a visible primary menu path.
  • merge-risk: 🚨 compatibility: The PR intentionally changes precise-trackpad Overview scrolling and selected-row rendering for existing users.
  • merge-risk: 🚨 availability: The PR changes NSMenu row hosting, layer filtering, and scroll event handling in the menu surface reported to stutter.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦞 diamond lobster.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: A baseline recording, a later short video, and benchmark/test output are present, but no inspected artifact identifies a freshly packaged after-fix build and covers scrolling plus interaction behavior. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • proof: 🎥 video: Contributor real behavior proof includes video or recording evidence. A baseline recording, a later short video, and benchmark/test output are present, but no inspected artifact identifies a freshly packaged after-fix build and covers scrolling plus interaction behavior.
Evidence reviewed

What I checked:

Likely related people:

  • joshuavial: Local history shows this person added the current Overview scroll-wheel highlight navigation path and tests that this PR changes for precise devices. (role: Overview scroll feature contributor; confidence: high; commits: 3c2d23d1739a; files: Sources/CodexBar/StatusItemController+OverviewScroll.swift, Tests/CodexBarTests/StatusMenuOverviewScrollTests.swift)
  • steipete: This PR includes the latest maintainer appearance pass, and history ties the current menu-hosting area to recent release and earlier menu highlight work by Peter Steinberger. (role: recent reviewer and adjacent contributor; confidence: high; commits: 534a9d0473c5, f380287041b8, a9fd4db8e27a; files: Sources/CodexBar/MenuCardGPUSelectionView.swift, Sources/CodexBar/StatusItemController+OverviewScroll.swift, Sources/CodexBar/StatusItemController+MenuPresentation.swift)
  • hhh2210: Beyond authoring this PR, local history shows prior merged menu performance work in nearby menu-card code. (role: adjacent merged menu performance contributor and PR author; confidence: medium; commits: b6d032702f28, 1d84a5994bb7, 65e39f4dcb3a; files: Sources/CodexBar/MenuCardGPUSelectionView.swift, Sources/CodexBar/StatusItemController+OverviewScroll.swift, docs/overview-scroll-stutter-investigation.md)
  • bcssewl: Local history shows this person introduced menu-card hosting view recycling and in-place menu reconciliation in the same custom NSMenu surface. (role: menu-card recycling contributor; confidence: medium; commits: f927e8ad90ae; files: Sources/CodexBar/StatusItemController+MenuCardItems.swift, Sources/CodexBar/StatusItemController+MenuPresentation.swift, Tests/CodexBarTests/MenuCardViewRecyclingTests.swift)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Copilot AI reviewed Jun 24, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR targets the Overview-menu scroll stutter by removing per-highlight SwiftUI invalidation work during NSMenu tracking, shifting selection rendering to an AppKit/CoreAnimation + Core Image (GPU) path and improving scroll “hand-feel” by letting precise trackpad scrolling use native menu scrolling.

Changes:

  • Add GPUSelectionHostingView to render selection background via NSVisualEffectView(.selection) and tint content via a Core Image filter, without toggling SwiftUI highlight state.
  • Update Overview scroll handling to pass precise scrolling deltas through to AppKit (native continuous scrolling), while keeping coarse wheel step navigation.
  • Extend test coverage for precise-vs-coarse scrolling behavior and for verifying GPU selection bypasses SwiftUI highlight state; add an investigation write-up doc.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
Tests/CodexBarTests/StatusMenuOverviewScrollTests.swift Updates scroll navigation tests to distinguish coarse wheel stepping vs precise pass-through behavior.
Tests/CodexBarTests/MenuCardViewRecyclingTests.swift Adds a regression test proving GPU-highlight does not flip SwiftUI highlight state.
Sources/CodexBar/StatusItemController+OverviewScroll.swift Passes precise scrolling through to AppKit; keeps wheel-based step navigation behavior.
Sources/CodexBar/StatusItemController+MenuCardItems.swift Adds usesGPUSelection option and wires GPU hosting view creation for menu card items.
Sources/CodexBar/StatusItemController+Menu.swift Enables GPU selection for Overview rows via usesGPUSelection: true.
Sources/CodexBar/MenuCardGPUSelectionView.swift Introduces the GPU/AppKit selection hosting view implementation.
docs/overview-scroll-stutter-investigation.md Adds detailed investigation notes, measurements, and rationale for the chosen approach.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread Sources/CodexBar/MenuCardGPUSelectionView.swift Outdated
Comment thread Sources/CodexBar/StatusItemController+OverviewScroll.swift Outdated
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 24, 2026
@hhh2210 @claude
refactor: address Copilot review on GPU selection tint and scroll com…
1d84a59 
…ment

- Derive the selection tint bias from NSColor.selectedMenuItemTextColor
  instead of hard-coding pure white, so graphite/high-contrast/accessibility
  appearances tint correctly and the code matches its own doc comment.
- Clarify the momentum-phase guard comment: precise trackpad/Magic Mouse
  scrolling already returns earlier, so the guard only covers non-precise
  devices that still report a momentum phase.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@hhh2210

hhh2210 commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels Jun 24, 2026
@hhh2210

hhh2210 commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author
CleanShot.2026-06-25.at.10.40.40.mp4

@clawsweeper re-review

@clawsweeper clawsweeper Bot added the proof: 🎥 video Contributor real behavior proof includes video or recording evidence. label Jun 25, 2026
@steipete

steipete commented Jun 28, 2026

Copy link
Copy Markdown
Owner

Maintainer pass pushed at 534a9d0. It refreshes the Core Image selection tint whenever effective appearance changes and resolves selected text color inside that appearance, covering dark/light, graphite, and accessibility transitions. Repository checks pass; all 21 focused recycling/highlight tests pass. @codex review

@steipete

steipete commented Jun 28, 2026

Copy link
Copy Markdown
Owner

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 28, 2026 •
edited
Loading

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jun 28, 2026
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Jun 28, 2026

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. You're on a roll.

Reviewed commit: 534a9d0473

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jun 28, 2026
@steipete steipete merged commit 627b1fb into steipete:main Jun 28, 2026
10 checks passed
This was referenced Jun 28, 2026
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@steipete steipete steipete left review comments
Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: 🎥 video Contributor real behavior proof includes video or recording evidence. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hhh2210 @steipete